Create a proper MSBuild ToolTask based VSTestTask by mcartoixa · Pull Request #2702 · microsoft/vstest

Mac (mcartoixa) · 2021-01-16T13:38:15Z

Description

Update from nohwnd:

A new task VSTestTask2 and a logger MSBuildLogger (for internal use only) was added. And can be enabled by setting /p:VsTestUseMSBuildOutput=true.

This is not enabled anywhere by default, neither in dotnet test, or under vstest, because I want to collect some feedback first.

The current console forwarding task has been renamed VSTestForwardingTask. The new VSTestTask, based on ToolTask, properly integrates into the MSBuild framework.
If the $(VSTestUseConsole) property is set to True during the build, the old console forwarding VSTestForwardTask is used: logs are lost but output is colorized (like today). Otherwise the new VSTestTask is used: logs are fine but colorization is lost (except for errors, that are properly recognized by MSBuild).

A new PR will be submitted right away on the dotnet/sdk project so that dotnet test sets the $(VSTestUseConsole) property.

In effect:

users that favor a colorized console output can use dotnet test (like today).
users that favor a proper MSBuild integration can use dotnet msbuild -Target:VSTest.

This PR feels more like a workaround than a proper fix: it still feels weird to depend on a MSBuild task (VSTestForwardingTask) that is not entirely MSBuild friendly... But it seems like the easiest fix that is compatible with the old behavior.
A proper fix would probably center around the implementation of dotnet test which could call dotnet exec vstest.console.dll directly (in addition to MSBuild, for the build part only). This would render current workarounds like for dotnet/msbuild#3130, or AllowFailureWithoutError definitely obsolete.

Related issue

Fixes (or at least gives an option around) #680, #1503, #2224, dotnet/sdk#9481, dotnet/msbuild#5387 and probably more...
This is a new attempt after #2437.

ghost · 2021-02-06T08:49:06Z

Please note that I would be happy to answer any questions and/or reservations, and/or discuss any suggestions or improvements on this PR.

ghost · 2021-02-06T22:21:48Z

Thank you for the contribution Mac (@mcartoixa), I'll check this during the week and ask my questions, if any.

ghost · 2021-02-06T22:22:10Z

/azp run

ghost · 2021-02-06T22:22:19Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2021-05-26T08:03:11Z

I just rebased this PR against the main branch so that it would not be an excuse for not merging it 😉

ghost · 2021-06-30T01:53:10Z

+
+                activeProcess.WaitForExit();
+                Tracing.Trace("VSTest: Exit code: " + activeProcess.ExitCode);
+                return activeProcess.ExitCode == 0;


We should only return !Log.HasLoggedErrors. The task should fail if and only if it has logged any errors.

I believe this is the way it's usually done in MSBuild tasks. Does vstest.console.exe return an error code even in cases of success?
Anyway this task is a copy (mostly left unchanged) of the current one, kept here for compatibility purposes. The goal of this PR is rather to introduce a new task that would be more respectful of MSBuild, see VSTestTask (the diff is rather useless on that one, though).

ghost · 2021-06-30T01:54:47Z

+
+            if (!string.IsNullOrEmpty(this.VSTestFramework))
+            {
+                Console.WriteLine(Resources.TestRunningSummary, this.TestFileFullPath, this.VSTestFramework);


A task shouldn't use Console.WriteLine, but instead use Log.LogMessage:
https://source.dot.net/#Microsoft.Build.Tasks.Core/Copy.cs,255

Agreed in general, but this has been left unchanged for compatibility with current behavior. The goal of this PR is rather to introduce a new task that would be more respectful of MSBuild, see VSTestTask.

ghost · 2021-06-30T01:55:23Z

+            // Avoid logging "Task returned false but did not log an error." on test failure, because we don't
+            // write MSBuild error. https://github.com/dotnet/msbuild/blob/51a1071f8871e0c93afbaf1b2ac2c9e59c7b6491/src/Framework/IBuildEngine7.cs#L12
+            var allowfailureWithoutError = BuildEngine.GetType().GetProperty("AllowFailureWithoutError");
+            allowfailureWithoutError?.SetValue(BuildEngine, true);


we should really not do this. If there was an error, log an MSBuild error. If there wasn't an error, don't log it.

Let's remove this reflection logic.

This has been left unchanged for compatibility with current behavior. The goal of this PR is rather to introduce a new task that would be more respectful of MSBuild, see VSTestTask.

ghost · 2021-06-30T03:57:50Z

Just to make it clear to me. The VSTestUseConsole build property is introduced by this PR, which has not been merged yet, right?

ghost · 2021-06-30T04:49:54Z

MarkKharitonov Right.
And to ensure compatibility would require the reopening of dotnet/sdk#15376 to initialize the VSTestUseConsole property in the dotnet test command.

ghost · 2021-10-24T07:52:05Z

Just rebased again against main (like I seem to do every 5 months or so) just in case somebody suddenly were to be interested in getting this PR merged. 😉

ghost · 2022-02-08T07:26:49Z

When can we expect some kind of decision about the future of this PR? As mentioned before quite a few bugs could be affected by this.

Is it going to be rejected, and why?
Is it going to be merged, and when?

ghost · 2022-04-22T19:37:56Z

It took me 2 days to set up a new barely working development environment, but I think I managed to rebase this PR against main. Again.

ghost · 2022-06-09T13:27:26Z

Any update on the status of this request?

ghost · 2022-06-28T07:48:32Z

nit: file should be renamed too!

test/Microsoft.TestPlatform.Build.UnitTests/VsTestTaskTests.cs -> VSTestTaskTests.cs

ghost

Overall I like it. It's a welcome change but a huge breaking change nonetheless, so its up to the owners, I think!

ghost · 2022-06-28T08:34:49Z

Nirmal Guru (@Nirmal4G) Thanks for the review! There is nothing more that I would like than having a discussion with the owners.

Meanwhile it seems that I will have to rebase this PR again...

Mac (mcartoixa) mentioned this pull request Jan 16, 2021

Add the $(VSTestUseConsole) property to the dotnet test command dotnet/sdk#15376

Closed

Mac (mcartoixa) mentioned this pull request May 17, 2021

dotnet test is incompatible with MSBuild node reuse #1503

Closed

Mac (mcartoixa) force-pushed the vstesttask-complete branch 2 times, most recently from 569e421 to cefe13d Compare May 26, 2021 06:40

Jakub Jareš (nohwnd) mentioned this pull request Jun 29, 2021

Better fix for MSBuild task failed but did not log an error #2953

Closed

Kirill Osenkov (KirillOsenkov) reviewed Jun 30, 2021

View reviewed changes

Mac (mcartoixa) force-pushed the vstesttask-complete branch from cefe13d to 7acb932 Compare October 24, 2021 07:47

Mac (mcartoixa) mentioned this pull request Feb 8, 2022

VSTestTask only logs to the console #680

Closed

Mac (mcartoixa) force-pushed the vstesttask-complete branch from 7acb932 to b6dcdc8 Compare April 22, 2022 19:35

Mac (mcartoixa) requested review from Marco Rossignoli (MarcoRossignoli) and Jakub Jareš (nohwnd) as code owners April 22, 2022 19:35

Mac (mcartoixa) force-pushed the vstesttask-complete branch 2 times, most recently from d1e30d4 to 1b4b548 Compare April 22, 2022 20:00

Jakub Jareš (nohwnd) self-assigned this Apr 22, 2022

Nirmal Guru (Nirmal4G) approved these changes Jun 28, 2022

View reviewed changes

Uh oh!

Conversation

Mac (mcartoixa) commented Jan 16, 2021 • edited by nohwnd Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issue

Uh oh!

ghost commented Feb 6, 2021

Uh oh!

ghost commented Feb 6, 2021

Uh oh!

ghost commented Feb 6, 2021

Uh oh!

ghost commented Feb 6, 2021

Uh oh!

ghost commented May 26, 2021 • edited by mcartoixa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

ghost Jun 30, 2021 • edited by mcartoixa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

ghost Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

ghost Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

ghost Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 30, 2021

Uh oh!

ghost commented Jun 30, 2021

Uh oh!

ghost commented Oct 24, 2021

Uh oh!

ghost commented Feb 8, 2022

Uh oh!

ghost commented Apr 22, 2022 • edited by mcartoixa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 9, 2022

Uh oh!

ghost commented Jun 28, 2022

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Mac (mcartoixa) commented Jan 16, 2021 •

edited by nohwnd

Loading

ghost commented May 26, 2021 •

edited by mcartoixa

Loading

ghost Jun 30, 2021 •

edited by mcartoixa

Loading

ghost commented Apr 22, 2022 •

edited by mcartoixa

Loading